Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CreateDirectory: eliminate some syscalls. #58799

Merged
merged 4 commits into from
Nov 18, 2021
Merged

CreateDirectory: eliminate some syscalls. #58799

merged 4 commits into from
Nov 18, 2021

Conversation

tmds
Copy link
Member

@tmds tmds commented Sep 8, 2021

This eliminates three syscalls per Directory.CreateDirectory when the path doesn't exist.

One is the initial check if the directory exist.

The other two are eliminating by using mkdir to check parent directory existence.
Instead of finding the first parent that exist using stat, we now end up creating the first parent that doesn't exist.

When Directory.CreateDirectory is called with a path that does exist, we are now making two syscalls instead of one.
If we want, we can avoid that regression by keeping the initial existence check. That costs us a syscall in the non-exists case.
A user can also avoid it by calling Directory.Exists before calling Directory.CreateDirectory.

Similar changes can be made to the Windows implementation.

@adamsitnik @stephentoub ptal.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 8, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO and removed community-contribution Indicates that the PR has been added by a community member labels Sep 8, 2021
@ghost
Copy link

ghost commented Sep 8, 2021

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

This eliminates three syscalls per Directory.CreateDirectory when the path doesn't exist.

One is the initial check if the directory exist.

The other two are eliminating by using mkdir to check parent directory existence.
Instead of finding the first parent that exist using stat, we now end up creating the first parent that doesn't exist.

When Directory.CreateDirectory is called with a path that does exist, we are now making two syscalls instead of one.
If we want, we can avoid that regression by keeping the initial existence check. That costs us a syscall in the non-exists case.
A user can also avoid it by calling Directory.Exists before calling Directory.CreateDirectory.

Similar changes can be made to the Windows implementation.

@adamsitnik @stephentoub ptal.

Author: tmds
Assignees: -
Labels:

area-System.IO
Milestone: -

jeffhandley
jeffhandley approved these changes Oct 10, 2021
View reviewed changes
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I'd like a secondary review from @dotnet/area-system-io.

@jeffhandley jeffhandley added the community-contribution Indicates that the PR has been added by a community member label Nov 15, 2021
adamsitnik
adamsitnik reviewed Nov 17, 2021
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but it would be great to refactor the code a little bit and if it's safe replace List<string> with List<int>

@tmds thank you for another great contribution!

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated Show resolved Hide resolved
@tmds
Copy link
Member Author

tmds commented Nov 18, 2021

@adamsitnik thanks for the review! I've addressed your comments.
I kept the List<string> since I need a string to pass to the MkDir PInvoke.

adamsitnik
adamsitnik approved these changes Nov 18, 2021
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, again thank you @tmds !

@adamsitnik adamsitnik merged commit 37bf145 into dotnet:main Nov 18, 2021
@adamsitnik adamsitnik added the tenet-performance Performance related issue label Nov 18, 2021
@adamsitnik adamsitnik added this to the 7.0.0 milestone Nov 18, 2021
@carlossanlop
Copy link
Member

@tmds @adamsitnik just for completeness, can we get perf numbers before and after this change?

@runfoapp runfoapp bot mentioned this pull request Nov 18, 2021
Closed
@tmds
Copy link
Member Author

tmds commented Nov 19, 2021

can we get perf numbers before and after this change?

Is there some place we can see the diff on the continuous benchmarking of the dotnet/performance repo?

@adamsitnik
Copy link
Member

Is there some place we can see the diff on the continuous benchmarking of the dotnet/performance repo?

https://pvscmdupload.blob.core.windows.net/reports/allTestHistory/TestHistoryIndexIndex.html

But the existing Directory APIs benchmarks are very noisy:

https://pvscmdupload.blob.core.windows.net/reports/allTestHistory%2frefs%2fheads%2fmain_x64_ubuntu%2018.04%2fSystem.IO.Tests.Perf_Directory.CreateDirectory.html
https://pvscmdupload.blob.core.windows.net/reports/allTestHistory%2frefs%2fheads%2fmain_x64_ubuntu%2018.04%2fSystem.IO.Tests.Perf_Directory.RecursiveCreateDeleteDirectory(depth%3a%2010).html
https://pvscmdupload.blob.core.windows.net/reports/allTestHistory%2frefs%2fheads%2fmain_x64_ubuntu%2018.04%2fSystem.IO.Tests.Perf_Directory.RecursiveCreateDeleteDirectory(depth%3a%20100).html

@tmds
Copy link
Member Author

tmds commented Nov 19, 2021 •
edited
Loading

The change wasn't run by the benchmarks yet. Let's see if we can see it in a couple of days.

@adamsitnik what generates this website? Is it part of BenchmarkDotNet?

@adamsitnik
Copy link
Member

The change wasn't run by the benchmarks yet

It's strange, our automation is supposed to run at least few times a day.

what generates this website? Is it part of BenchmarkDotNet?

it's closed source tool maintained by @DrewScoggins

@DrewScoggins
Copy link
Member

Long story short the data on the test history index page only updates about once a day, because it takes about a day to generate. I am looking to make some changes to greatly reduce the time it takes to generate them, but for now that is where we are. As a result they can sometimes be a little behind.

I went ahead and generated a report for just the Directory tests, and though one is marked as a regression, it looks like just noise in the test. I don't see any significant regression or improvement as the result of this change. Report

@tmds
Copy link
Member Author

tmds commented Nov 22, 2021

By now, the benchmark ran a few times with the changes.
newplot

@danmoseley
Copy link
Member

do we need an up for grabs issue to do the same for Windows?

@tmds tmds mentioned this pull request Nov 23, 2021
Open
@tmds
Copy link
Member Author

tmds commented Nov 23, 2021

do we need an up for grabs issue to do the same for Windows?

I've created #61954.

@danmoseley
Copy link
Member

thanks.

This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks left review comments

@danmoseley danmoseley danmoseley left review comments

@iSazonov iSazonov iSazonov left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

@adamsitnik adamsitnik adamsitnik approved these changes

Assignees

@adamsitnik adamsitnik

Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member tenet-performance Performance related issue
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@tmds @carlossanlop @adamsitnik @DrewScoggins @danmoseley @jeffhandley @GrabYourPitchforks @iSazonov